iscsi session cleanup now configurable, filters iscsi partitions by skattoju4 · Pull Request #4219 · apache/cloudstack

skattoju4 · 2020-07-20T20:51:41Z

Description

Added property to agent.properties that enables or disables the iscsi session clean up feature. #4210
Added a condition to prevent disk partitions from being cleaned up. #4216

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

GabrielBrascher

Thanks for the PR, @skattoju4. I have added a few observations.

I do not have any test environment with iSCSI; therefore, I will not be able to test this one. My contributions will be only regarding code enhancements.

agent/conf/agent.properties

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

GabrielBrascher · 2020-07-21T08:54:23Z

...pervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java

                    //check the volume map. If an entry exists change the status to True
                    for (final LibvirtVMDef.DiskDef disk : disks) {
-                        if (diskStatusMap.containsKey(disk.getDiskPath())) {
+                        if (diskStatusMap.containsKey(disk.getDiskPath())&&!disk.getDiskPath().contains("part")) {


Can you please extract "part" to a constant?
Adding some details/documentation on why it is important to have this constant checked might be a plus as well.

@svenvogel @skattoju4 The commit from PR #3819 was added on 4.13, right? This PR should then be also aimed at branch 4.13.

would you know whats the best way to rebase it to 4.13 is ?

GabrielBrascher · 2020-07-21T13:28:50Z

@svenvogel @skattoju4 The commit from PR #3819 was added on 4.13, right? This PR should then be also aimed at branch 4.13.

RodrigoDLopez · 2020-07-21T16:07:58Z

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

+            final Thread cleanupMonitor = new Thread(isciCleanupMonitor);
+            cleanupMonitor.start();
+        } else {
+            s_logger.info("iscsi session clean up is disabled")


I believe you have forgotten the ';' on here.

svenvogel · 2020-07-21T22:26:30Z

@GabrielBrascher yes sure its in 4.13.1 and 4.14 and can be rebased. how is it with a forward merge after merge?

mike-tutkowski · 2020-07-22T16:46:53Z

...pervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java

    private static final String ISCSI_PATH_PREFIX = "/dev/disk/by-path";
    private static final String KEYWORD_ISCSI = "iscsi";
    private static final String KEYWORD_IQN = "iqn";
+    private static final String KEYWORD_PART = "part";


Is this constant used anywhere?

mike-tutkowski · 2020-07-22T16:47:32Z

...pervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/IscsiStorageCleanupMonitor.java

    private static final String KEYWORD_ISCSI = "iscsi";
    private static final String KEYWORD_IQN = "iqn";
+    private static final String KEYWORD_PART = "part";
+    private static final String REGEX_PART = "\\S+part\\d+$";


Perhaps we want to use KEYWORD_PART here instead of "part"?

mike-tutkowski

I have not personally tested this code, but it LGTM.

svenvogel · 2020-07-24T18:39:41Z

@skattoju4 yes we will test this and come back as fast as we can. 👍

smartsoftwareSD · 2020-07-27T14:49:55Z

I manually tested the code over the weekend and it seems to work as expected. My test deployment uses unmanaged ISCSI connections underlying a CLVM Volume Group for its Primary Storage. Here are the three test cases and results.

Add iscsi.session.cleanup.enabled=false to agent.properties. Agent initializes and operates as expected. No effect on unmanaged ISCSI connections occurs.
Add iscsi.session.cleanup.enabled=true to agent.properties. Unmanaged connections are deleted when the agent starts up causing the node to fail due to loss of access to the primary storage.
Remove iscsi.session.cleanup.enabled setting from agent.properties. Agent initializes and operates as expected. No effect on unmanaged ISCSI connections occurs.

Looks good to me.

GabrielBrascher

Thanks for the updates, @skattoju4.
LGTM based on code review.

GabrielBrascher · 2020-08-05T19:58:36Z

@svenvogel @skattoju4 regarding the question of which branch this one should be targeted:
Considering that the bug was introduced in 4.13 and we still support 4.13 as LTS, I think that this one should be merged on 4.13 and then merge-forwarded to 4.14, and master.

kiwiflyer · 2020-08-05T20:18:06Z

@GabrielBrascher yes, I agree.

yadvr · 2020-08-13T10:51:55Z

@blueorangutan package

blueorangutan · 2020-08-13T10:52:57Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

yadvr · 2020-08-13T10:53:57Z

@skattoju4 can you cherry-pick or rebase your PR branch against 4.13?

yadvr · 2020-08-14T07:37:36Z

@skattoju4 it's not just changing the base branch - see the diff - you need to rebase changes against 4.13 (base branch)

This reverts commit 26776ad.

yadvr · 2020-08-20T09:35:30Z

@blueorangutan package

blueorangutan · 2020-08-20T09:36:09Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2020-08-20T09:57:11Z

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-1792

yadvr · 2020-08-20T11:52:52Z

@blueorangutan test

blueorangutan · 2020-08-20T11:54:13Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2020-08-20T22:01:18Z

Trillian test result (tid-2520)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34346 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4219-t2520-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 75 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_vpc_privategw_static_routes	`Failure`	201.89	test_privategw_acl.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	199.16	test_privategw_acl.py
test_04_rvpc_privategw_static_routes	`Failure`	296.18	test_privategw_acl.py
test_01_redundant_vpc_site2site_vpn	`Failure`	397.70	test_vpc_vpn.py

svenvogel added component:managed-storage component:primary-storage type:bug labels Jul 21, 2020

svenvogel requested review from GabrielBrascher and mike-tutkowski July 21, 2020 08:23

svenvogel added this to the 4.15.0.0 milestone Jul 21, 2020

GabrielBrascher reviewed Jul 21, 2020

View reviewed changes

RodrigoDLopez reviewed Jul 21, 2020

View reviewed changes

skattoju4 changed the base branch from master to 4.13 July 22, 2020 14:27

skattoju4 changed the base branch from 4.13 to master July 22, 2020 14:28

mike-tutkowski reviewed Jul 22, 2020

View reviewed changes

mike-tutkowski approved these changes Jul 22, 2020

View reviewed changes

svenvogel requested a review from GabrielBrascher July 30, 2020 14:07

GabrielBrascher approved these changes Aug 5, 2020

View reviewed changes

kiwiflyer modified the milestones: 4.15.0.0, 4.13.2.0 Aug 5, 2020

yadvr changed the base branch from master to 4.13 August 13, 2020 10:53

yadvr changed the base branch from 4.13 to 4.14 August 13, 2020 10:53

yadvr changed the base branch from 4.14 to master August 13, 2020 10:53

skattoju4 changed the base branch from master to 4.13 August 13, 2020 15:11

skattoju4 added 7 commits August 14, 2020 12:17

resolved conflict

a668d04

resolved conflict in clean up monitor

e0c6864

resolved conflict in agent properties

64bf2aa

use regex instead of contains

b0e75cf

remove excess \

9a36abd

Revert "remove excess \"

c4011df

This reverts commit 26776ad.

import BooleanUtils removed unused constant

7f34c9e

skattoju4 force-pushed the iscsi_cleanup_bug_fix branch from 7697ad6 to 7f34c9e Compare August 14, 2020 16:24

yadvr added the Severity:Major label Aug 20, 2020

yadvr merged commit 1da76d2 into apache:4.13 Aug 21, 2020

This was referenced Aug 25, 2020

iSCSI paths will be logged out / path detection failure #4216

Closed

ISCSI cleanup thread breaks non-managed storage utilizing ISCSI connectivity #4210

Closed

Conversation

skattoju4 commented Jul 20, 2020

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

GabrielBrascher left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

GabrielBrascher Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

skattoju4 Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RodrigoDLopez Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

skattoju4 Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

svenvogel commented Jul 21, 2020

Uh oh!

mike-tutkowski Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

skattoju4 Jul 23, 2020

Choose a reason for hiding this comment

Uh oh!

mike-tutkowski Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mike-tutkowski left a comment

Choose a reason for hiding this comment

Uh oh!

svenvogel commented Jul 24, 2020

Uh oh!

smartsoftwareSD commented Jul 27, 2020

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Aug 5, 2020

Uh oh!

kiwiflyer commented Aug 5, 2020

Uh oh!

yadvr commented Aug 13, 2020

Uh oh!

blueorangutan commented Aug 13, 2020

Uh oh!

yadvr commented Aug 13, 2020

Uh oh!

yadvr commented Aug 14, 2020

Uh oh!

yadvr commented Aug 20, 2020

Uh oh!

blueorangutan commented Aug 20, 2020

Uh oh!

blueorangutan commented Aug 20, 2020

Uh oh!

yadvr commented Aug 20, 2020

Uh oh!

blueorangutan commented Aug 20, 2020

Uh oh!

blueorangutan commented Aug 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

GabrielBrascher left a comment •

edited

Loading

GabrielBrascher commented Jul 21, 2020 •

edited

Loading

mike-tutkowski Jul 22, 2020 •

edited

Loading